Skip to content

implement alter default privileges#2782

Open
jennifersp wants to merge 1 commit into
mainfrom
jennifer/dp
Open

implement alter default privileges#2782
jennifersp wants to merge 1 commit into
mainfrom
jennifer/dp

Conversation

@jennifersp
Copy link
Copy Markdown
Contributor

No description provided.

@github-actions
Copy link
Copy Markdown
Contributor

Main PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

Main PR
Total 42090 42090
Successful 18202 18209
Failures 23888 23881
Partial Successes1 5374 5372
Main PR
Successful 43.2454% 43.2621%
Failures 56.7546% 56.7379%

${\color{red}Regressions (2)}$

psql

QUERY:          SELECT pg_catalog.pg_get_userbyid(d.defaclrole) AS "Owner",
  n.nspname AS "Schema",
  CASE d.defaclobjtype WHEN 'r' THEN 'table' WHEN 'S' THEN 'sequence' WHEN 'f' THEN 'function' WHEN 'T' THEN 'type' WHEN 'n' THEN 'schema' END AS "Type",
  pg_catalog.array_to_string(d.defaclacl, E'\n') AS "Access privileges"
FROM pg_catalog.pg_default_acl d
     LEFT JOIN pg_catalog.pg_namespace n ON n.oid = d.defaclnamespace
WHERE (n.nspname OPERATOR(pg_catalog.~) E'^(no\\.such\\.default\\.access\\.privilege)$' COLLATE pg_catalog.default
        OR pg_catalog.pg_get_userbyid(d.defaclrole) OPERATOR(pg_catalog.~) E'^(no\\.such\\.default\\.access\\.privilege)$' COLLATE pg_catalog.default)
ORDER BY 1, 2, 3;
RECEIVED ERROR: cast from type oid to type oid is mislabeled as binary-coercible (errno 1105) (sqlstate HY000)
QUERY:          SELECT pg_catalog.pg_get_userbyid(d.defaclrole) AS "Owner",
  n.nspname AS "Schema",
  CASE d.defaclobjtype WHEN 'r' THEN 'table' WHEN 'S' THEN 'sequence' WHEN 'f' THEN 'function' WHEN 'T' THEN 'type' WHEN 'n' THEN 'schema' END AS "Type",
  pg_catalog.array_to_string(d.defaclacl, E'\n') AS "Access privileges"
FROM pg_catalog.pg_default_acl d
     LEFT JOIN pg_catalog.pg_namespace n ON n.oid = d.defaclnamespace
WHERE (n.nspname OPERATOR(pg_catalog.~) E'^(no\\.such\\.schema.no\\.such\\.default\\.access\\.privilege)$' COLLATE pg_catalog.default
        OR pg_catalog.pg_get_userbyid(d.defaclrole) OPERATOR(pg_catalog.~) E'^(no\\.such\\.schema.no\\.such\\.default\\.access\\.privilege)$' COLLATE pg_catalog.default)
ORDER BY 1, 2, 3;
RECEIVED ERROR: cast from type oid to type oid is mislabeled as binary-coercible (errno 1105) (sqlstate HY000)

${\color{lightgreen}Progressions (9)}$

dependency

QUERY: ALTER DEFAULT PRIVILEGES FOR ROLE regress_dep_user1 IN SCHEMA deptest
  GRANT ALL ON TABLES TO regress_dep_user2;

event_trigger

QUERY: alter default privileges for role regress_evt_user
 revoke delete on tables from regress_evt_user;

matview

QUERY: ALTER DEFAULT PRIVILEGES FOR ROLE regress_matview_user
  REVOKE INSERT ON TABLES FROM regress_matview_user;
QUERY: ALTER DEFAULT PRIVILEGES FOR ROLE regress_matview_user
  GRANT INSERT ON TABLES TO regress_matview_user;

object_address

QUERY: ALTER DEFAULT PRIVILEGES FOR ROLE regress_addr_user IN SCHEMA public GRANT ALL ON TABLES TO regress_addr_user;
QUERY: ALTER DEFAULT PRIVILEGES FOR ROLE regress_addr_user REVOKE DELETE ON TABLES FROM regress_addr_user;

random

QUERY: (SELECT unique1 AS random
  FROM onek ORDER BY random() LIMIT 1)
INTERSECT
(SELECT unique1 AS random
  FROM onek ORDER BY random() LIMIT 1)
INTERSECT
(SELECT unique1 AS random
  FROM onek ORDER BY random() LIMIT 1);

select_into

QUERY: ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
	  REVOKE INSERT ON TABLES FROM regress_selinto_user;
QUERY: ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user
	  GRANT INSERT ON TABLES TO regress_selinto_user;

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@itoqa
Copy link
Copy Markdown

itoqa Bot commented May 29, 2026

Ito Test Report ❌

20 test cases ran. 4 failed, 4 additional findings, 12 passed.

Across 20 test cases, 12 passed and 8 failed: default-privilege and object-inheritance flows largely behaved correctly (including grant/revoke variants, omitted FOR ROLE behavior, table/routine/sequence coverage, routine write-fault cleanup, and ATTACH/DETACH partition formatting). The key findings were several production defects led by high-severity auth durability/restore problems (non-atomic default-ACL mutation on persist failure, auth.db v2 restart panics, corruption-driven startup crashes, and v1-to-v2 upgrade restart failure), plus catalog stability issues (owner-filter lookups blocked by empty pg_roles and pg_default_acl OID type mismatch panics) and parser/diagnostic gaps (broken PRIMARY KEY USING INDEX parse/format path and generic ALTER INDEX unsupported errors).

❌ Failed (4)
Category Summary Screenshot
Catalog ⚠️ Global-scope pg_default_acl verification hit an OID type panic during catalog reads. CATALOG-3
Privilege ⚠️ ALTER DEFAULT PRIVILEGES mutates in-memory ACL state before persistence; failed writes can leave restart-divergent authorization behavior. PRIVILEGE-5
Serialization ⚠️ auth.db is written as v2 but restart deserializers panic on version 2 privilege payloads. N/A
Serialization ⚠️ v1-to-v2 upgrade flow can persist v2 state that panics on subsequent restart. SERIALIZATION-3
⚠️ OID typing mismatch crashes catalog reads
  • What failed: Catalog reads can panic with interface conversion: interface {} is uint32, not id.Id, preventing verification of expected global-scope rows (defaclnamespace = 0).
  • Impact: Queries against pg_default_acl can fail at runtime for valid introspection workflows, blocking visibility into default privilege state. This breaks a core catalog surface for authorization diagnostics.
  • Steps to reproduce:
    1. Execute global-scope ALTER DEFAULT PRIVILEGES FOR ROLE GRANT SELECT ON TABLES TO without IN SCHEMA.
    2. Query pg_catalog.pg_default_acl.
    3. Read OID-typed fields during projection or filtering.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: I traced pg_default_acl row construction and compared it to the OID type contract. The handler emits raw uint32 for OID columns, but the OID type serializer/deserializer expects id.Id; this mismatch explains the runtime interface-conversion panic.
  • Why this is likely a bug: The handler violates the declared OID runtime type contract (id.Id), which directly causes query-time type conversion failures in production code.

Relevant code:

server/tables/pgcatalog/pg_default_acl.go (lines 70-75)

rows = append(rows, sql.Row{
    uint32(oid + 1), // oid (synthetic, 1-based index)
    uint32(entry.Key.OwnerRole),
    uint32(0), // defaclnamespace: 0 = any schema (schema OID lookup not yet implemented)
    auth.DefaultPrivilegeObjTypeChar(entry.Key.ObjectType),
    aclItems,
})

server/types/oid.go (lines 61-74)

// serializeTypeOid handles serialization from the standard representation to our serialized representation that is
// written in Dolt.
func serializeTypeOid(ctx *sql.Context, t *DoltgresType, val any) ([]byte, error) {
    return []byte(val.(id.Id)), nil
}

// deserializeTypeOid handles deserialization from the Dolt serialized format to our standard representation used by
// expressions and nodes.
func deserializeTypeOid(ctx *sql.Context, t *DoltgresType, data []byte) (any, error) {
    if len(data) == 0 {
        return nil, nil
    }
    return id.Id(data), nil
}
⚠️ Default privilege mutation is not atomic with persistence
  • What failed: The statement returns a persistence error, but the in-memory default ACL mutation is already applied, so pre-restart behavior can reflect the new default while post-restart behavior (after reloading from durable state) does not.
  • Impact: Authorization outcomes can diverge across restart boundaries after a write failure, producing inconsistent access control decisions for the same role and object scope. Recovery can include restart instability when corrupted auth state is deserialized.
  • Steps to reproduce:
    1. Run ALTER DEFAULT PRIVILEGES while forcing persistence to fail at write time.
    2. In the same process, create a new object owned by the altered role and verify inherited privilege behavior.
    3. Restart the server and create another object for the same owner/scope to compare inherited privileges.
  • Stub / mock context: To exercise persistence-failure handling, the run temporarily forced the auth persistence target into a non-writable state so ALTER DEFAULT PRIVILEGES returned a write error, then restored normal writes for restart comparison.
  • Code analysis: I inspected default-privilege execution and auth serialization paths and found mutation occurs before PersistChanges, with no rollback when persistence fails. The same code path also writes auth state directly and panics on deserialize errors at startup, which matches the observed restart fragility under failed persistence scenarios.
  • Why this is likely a bug: The implementation commits default-ACL mutations to in-memory state before durability succeeds and does not revert on write failure, so authorization behavior can legitimately split between pre-restart memory and post-restart persisted state.

Relevant code:

server/node/alter_default_privileges.go (lines 64-73)

auth.LockWrite(func() {
	err = n.execute(ctx)
	if err != nil {
		return
	}
	err = auth.PersistChanges()
})
if err != nil {
	return nil, err
}

server/node/alter_default_privileges.go (lines 123-137)

for _, ownerRole := range ownerRoles {
	for _, schema := range schemas {
		key := auth.DefaultPrivilegeKey{
			OwnerRole:  ownerRole.ID(),
			Schema:     schema,
			ObjectType: n.ObjectType,
		}
		for _, granteeRole := range granteeRoles {
			for _, priv := range n.Privileges {
				grantedPrivilege := auth.GrantedPrivilege{
					Privilege: priv,
					GrantedBy: ownerRole.ID(),
				}
				if n.Grant {
					auth.AddDefaultPrivilege(key, granteeRole.ID(), grantedPrivilege, n.GrantOption)

server/auth/serialization.go (lines 25-28)

func PersistChanges() error {
	if fileSystem != nil {
		return fileSystem.WriteFile(authFileName, globalDatabase.serialize(), 0644)
	}

server/auth/database.go (lines 176-177)

} else if err = globalDatabase.deserialize(authData); err != nil {
				panic(err)
⚠️ Auth DB version 2 restart panic
  • What failed: Restart reads auth data as version 2, then panics while deserializing privilege maps that only support versions 0/1, so default privileges are not recoverable after restart.
  • Impact: Persisting default privileges can make the server fail to start on the next boot. This breaks a core auth workflow and has no practical runtime workaround once the file is written.
  • Steps to reproduce:
    1. Connect as superuser and configure default table privileges for an owner role.
    2. Restart the server process using persisted auth.db state.
    3. Create a new table as the owner role and verify reader-role access after restart.
  • Stub / mock context: Deterministic auth setup was used by bypassing SCRAM authentication in server/authentication_scram.go and bypassing domain constraint injection in server/analyzer/domain_constraints.go before running restart checks. This case also manipulated local auth.db files (backup and replacement) to validate restart behavior under persisted state transitions.
  • Code analysis: I reviewed auth serialization/deserialization flow and privilege map decoders. The serializer writes version 2 and deserializeV2 routes version 2 into legacy privilege deserializers that explicitly panic on unknown versions.
  • Why this is likely a bug: The code persists version 2 auth data but immediately routes version 2 reads into decoders that intentionally panic on that version, creating a deterministic restart failure path.

Relevant code:

server/auth/serialization.go (lines 36-55)

// Write the version
writer.Uint32(2)
// Write the database privileges
db.databasePrivileges.serialize(writer)
// Write the schema privileges
db.schemaPrivileges.serialize(writer)
// Write the table privileges
db.tablePrivileges.serialize(writer)
// Write the sequence privileges
db.sequencePrivileges.serialize(writer)
// Write the routine privileges
db.routinePrivileges.serialize(writer)
// Write the role chain
db.roleMembership.serialize(writer)
// Write the default privileges
db.defaultPrivileges.serialize(writer)

server/auth/serialization.go (lines 149-162)

// Read the database privileges
db.databasePrivileges.deserialize(2, reader)
// Read the schema privileges
db.schemaPrivileges.deserialize(2, reader)
// Read the table privileges
db.tablePrivileges.deserialize(2, reader)
// Read the sequence privileges
db.sequencePrivileges.deserialize(2, reader)
// Read the routine privileges
db.routinePrivileges.deserialize(2, reader)
// Read the role membership
db.roleMembership.deserialize(2, reader)

server/auth/database_privileges.go (lines 173-200)

switch version {
case 0, 1:
    // Read the total number of values
    dataCount := reader.Uint64()
    for dataIdx := uint64(0); dataIdx < dataCount; dataIdx++ {
        // Read the key
        spv := DatabasePrivilegeValue{Privileges: make(map[Privilege]map[GrantedPrivilege]bool)}
        spv.Key.Role = RoleID(reader.Uint64())
        spv.Key.Name = reader.String()
    }
default:
    panic("unexpected version in DatabasePrivileges")
}
⚠️ Upgrade path v1 to v2 break
  • What failed: After first v2 persistence, restart deserializes with version 2 while multiple privilege and membership decoders still only accept 0/1 and panic, breaking upgrade continuity.
  • Impact: A v1 environment can appear to upgrade but then fail on the first subsequent restart after v2 persistence. This risks post-upgrade outages in core authorization behavior.
  • Steps to reproduce:
    1. Start from a v1 auth.db fixture and run the newer binary.
    2. Execute ALTER DEFAULT PRIVILEGES and persist auth state so the file is rewritten as v2.
    3. Restart and verify inherited privileges for newly created objects.
  • Stub / mock context: Deterministic auth setup was used by bypassing SCRAM authentication in server/authentication_scram.go and bypassing domain constraint injection in server/analyzer/domain_constraints.go. This case also used a synthetic legacy auth.db variant to exercise the v1-to-v2 upgrade-and-restart path.
  • Code analysis: I inspected upgrade serialization routing and all downstream deserializers used by deserializeV2. Several maps still gate on versions 0/1 only and panic for version 2, so upgrade plus restart is structurally incompatible.
  • Why this is likely a bug: The upgrade flow writes version 2 data but restart decode paths still reject version 2 in multiple required structures, making the failure reproducible from source logic alone.

Relevant code:

server/auth/serialization.go (lines 149-162)

// Read the database privileges
db.databasePrivileges.deserialize(2, reader)
// Read the schema privileges
db.schemaPrivileges.deserialize(2, reader)
// Read the table privileges
db.tablePrivileges.deserialize(2, reader)
// Read the sequence privileges
db.sequencePrivileges.deserialize(2, reader)
// Read the routine privileges
db.routinePrivileges.deserialize(2, reader)
// Read the role membership
db.roleMembership.deserialize(2, reader)

server/auth/schema_privileges.go (lines 173-200)

switch version {
case 0, 1:
    // Read the total number of values
    dataCount := reader.Uint64()
    for dataIdx := uint64(0); dataIdx < dataCount; dataIdx++ {
        // Read the key
        spv := SchemaPrivilegeValue{Privileges: make(map[Privilege]map[GrantedPrivilege]bool)}
        spv.Key.Role = RoleID(reader.Uint64())
        spv.Key.Schema = reader.String()
    }
default:
    panic("unexpected version in SchemaPrivileges")
}

server/auth/role_membership.go (lines 141-165)

switch version {
case 0, 1:
    // Read the total number of members
    memberCount := reader.Uint64()
    for memberIdx := uint64(0); memberIdx < memberCount; memberIdx++ {
        // Read the number of groups
        groupCount := reader.Uint64()
        groupMap := make(map[RoleID]RoleMembershipValue)
        var member RoleID
    }
default:
    panic("unexpected version in RoleMembership")
}
✅ Passed (12)
Category Summary Screenshot
Catalog Verified pg_default_acl returns rows for configured TABLE and FUNCTION defaults. CATALOG-1
Catalog Verified large ACL arrays remain queryable with 20 ACL entries and valid aclitem formatting. CATALOG-4
Object Old table stayed denied; new table inherited default SELECT grant. OBJECT-1
Object EXECUTE stayed denied on old routines and allowed on newly created routines. OBJECT-2
Object SERIAL table path applied default privileges to both table SELECT and sequence usage. OBJECT-3
Object Forced auth persistence failure returned an explicit error and no callable routine remained. OBJECT-4
Parser Parser round-trip formatted ATTACH PARTITION with child_tbl (not parent_tbl). PARSER-1
Parser Both DETACH statements formatted with child_tbl and preserved CONCURRENTLY/FINALIZE modifiers. PARSER-2
Privilege Default table grant applied; grantee could read the newly created table. PRIVILEGE-1
Privilege Default privileges without FOR ROLE resolved to the active owner session and propagated to new tables. PRIVILEGE-2
Privilege REVOKE GRANT OPTION FOR preserved SELECT while full REVOKE removed SELECT for later tables. PRIVILEGE-3
Privilege REVOKE ... CASCADE was rejected and existing default ACL behavior remained unchanged. PRIVILEGE-4
ℹ️ Additional Findings (4)

These findings are unrelated to the current changes but were observed during testing.

Category Summary Screenshot
Catalog 🟠 Owner-targeted catalog verification could not retrieve matching rows to validate grant-option aclitem formatting. CATALOG-2
Parser 🟠 ALTER TABLE ... PRIMARY KEY USING INDEX fails to parse and formatter emits malformed USING INDEX output without separator spacing. PARSER-3
Parser ℹ️ ALTER INDEX ATTACH PARTITION diagnostics return a generic unsupported error that omits the referenced object names. PARSER-4
Serialization ⚠️ Corrupted auth.db bytes can trigger unchecked deserialization panics during startup. N/A
🟠 Owner filtered ACL catalog lookup fails
  • What failed: Owner-scoped validation could not find matching rows needed to confirm role-name and '*' grant-option formatting in defaclacl, even after successful ALTER DEFAULT PRIVILEGES execution.
  • Impact: Catalog-based tooling and diagnostics that rely on role-filtered default ACL inspection can report missing data even when defaults were configured. This weakens operator visibility into effective privilege state.
  • Steps to reproduce:
    1. Create owner and grantee roles.
    2. Run ALTER DEFAULT PRIVILEGES FOR ROLE GRANT SELECT ON TABLES TO WITH GRANT OPTION.
    3. Query pg_catalog.pg_default_acl using owner-scoped lookup patterns.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: I reviewed the catalog handlers used by owner-scoped checks. pg_default_acl data is emitted, but owner-name based filters depend on role catalog resolution. pg_roles currently returns an empty iterator, which makes owner lookup/join paths non-functional.
  • Why this is likely a bug: Returning an always-empty pg_roles catalog breaks legitimate owner-scoped catalog queries, so missing rows are caused by production code behavior rather than test setup.

Relevant code:

server/tables/pgcatalog/pg_roles.go (lines 45-48)

// RowIter implements the interface tables.Handler.
func (p PgRolesHandler) RowIter(ctx *sql.Context, partition sql.Partition) (sql.RowIter, error) {
    // TODO: Implement pg_roles row iter
    return emptyRowIter()
}
🟠 PRIMARY KEY USING INDEX parser and formatter defects
  • What failed: Expected the statement to parse and preserve app.orders_idx cleanly, but grammar accepts PRIMARY without KEY in this production and formatter emits USING INDEXapp.orders_idx without required spacing.
  • Impact: Schema migration workflows that rely on PRIMARY KEY USING INDEX can fail or emit malformed SQL text. Teams may need manual SQL rewrites to proceed.
  • Steps to reproduce:
    1. Parse ALTER TABLE app.orders ADD CONSTRAINT orders_pkey PRIMARY KEY USING INDEX app.orders_idx.
    2. Format the generated AST back into SQL.
    3. Observe parse failure around USING and formatter output that concatenates USING INDEX with the index name.
  • Stub / mock context: The run used a local parser harness and rendered deterministic SQL outputs in local browser data pages; no API route interception or bypass flag was used for this test. Temporary local startup patches were applied outside the parser object-name paths under test.
  • Code analysis: I reviewed parser grammar and tree formatting code in postgres/parser/parser/sql.y and postgres/parser/sem/tree/alter_table.go; the rule for this syntax omits KEY in the accepted production and the formatter concatenates the index name immediately after USING INDEX.
  • Why this is likely a bug: The production code directly encodes an incomplete grammar token sequence and malformed formatting output for a valid SQL form, so this is a concrete parser/formatter defect rather than test setup noise.

Relevant code:

postgres/parser/parser/sql.y (lines 2131-2134)

| ADD CONSTRAINT constraint_name unique_or_primary USING INDEX db_object_name opt_deferrable_mode opt_initially
  {
    $$.val = tree.AlterTableConstraintUsingIndex{Constraint: tree.Name($3), IsUnique: $4.bool(), Index: $7.unresolvedObjectName(), Deferrable: $8.deferrableMode(), Initially: $9.initiallyMode()}
  }

postgres/parser/parser/sql.y (lines 2360-2368)

unique_or_primary:
  UNIQUE
  {
    $$.val = true
  }
| PRIMARY
  {
    $$.val = false
  }

postgres/parser/sem/tree/alter_table.go (lines 423-424)

ctx.WriteString(" USING INDEX")
	node.Index.Format(ctx)
ℹ️ ALTER INDEX unsupported diagnostic omits object identifiers
  • What failed: Expected unsupported diagnostics to include identifier context for the referenced index and partition names, but execution always returns a generic message with no object names.
  • Impact: Unsupported-operation errors provide weak debugging context, which slows troubleshooting when multiple index operations are being tested. Core parser behavior still works, but diagnostics are less actionable.
  • Steps to reproduce:
    1. Parse ALTER INDEX app.parent_idx ATTACH PARTITION app.child_idx.
    2. Format the AST and confirm qualified object names are preserved.
    3. Trigger the unsupported execution conversion and inspect the returned error text.
  • Stub / mock context: The run used a local parser harness and rendered deterministic SQL outputs in local browser data pages; no API route interception or bypass flag was used for this test. Temporary local startup patches were applied outside the parser object-name paths under test.
  • Code analysis: I reviewed parse/format code and the AST conversion path. Parser and formatter preserve qualified partition index names, but server/ast/alter_index.go returns a fixed unsupported message regardless of input identifiers.
  • Why this is likely a bug: The unsupported error path discards parsed object identity even though the AST preserves it, causing diagnosability loss in production error reporting.

Relevant code:

postgres/parser/parser/sql.y (lines 2029-2031)

| ALTER INDEX table_index_name ATTACH PARTITION db_object_name
  {
    $$.val = &tree.AlterIndex{Index: $3.tableIndexName(), Cmd: &tree.AlterIndexAttachPartition{Index: $6.unresolvedObjectName()}}
  }

postgres/parser/sem/tree/alter_index.go (lines 99-102)

func (node *AlterIndexAttachPartition) Format(ctx *FmtCtx) {
	ctx.WriteString(" ATTACH PARTITION ")
	node.Index.Format(ctx)
}

server/ast/alter_index.go (lines 29-30)

// Only PARTITION alterations are supported by the parser, so there's nothing to convert to yet
	return NotYetSupportedError("ALTER INDEX is not yet supported")
⚠️ Corrupted auth database startup crash
  • What failed: Corrupted bytes can propagate into unchecked reader operations and deserialize errors that are escalated via panic, crashing startup instead of handling corruption safely.
  • Impact: A damaged auth file can take the service down during boot. Operators get a crash path rather than controlled failure handling or safe recovery.
  • Steps to reproduce:
    1. Create non-empty default privileges and persist auth state to auth.db.
    2. Truncate or corrupt the persisted auth.db bytes.
    3. Restart the service and observe startup behavior during auth deserialize.
  • Stub / mock context: Deterministic auth setup was used by bypassing SCRAM authentication in server/authentication_scram.go and bypassing domain constraint injection in server/analyzer/domain_constraints.go. This case intentionally truncated local auth.db bytes to simulate storage corruption before restart.
  • Code analysis: I traced startup load and low-level decoding. Startup panics directly on deserialize errors, and the reader APIs index and slice the buffer without bounds checks, allowing malformed data to trigger runtime panics.
  • Why this is likely a bug: Production startup code can panic on malformed persisted auth data instead of failing safely, which is a real reliability defect in core boot-time auth loading.

Relevant code:

server/auth/database.go (lines 165-177)

authData, err := fileSystem.ReadFile(authFileName)
if os.IsNotExist(err) {
    dbInitDefault()
    if err = dbInitCreateAuthDirectory(authFileName); err != nil {
        panic(err)
    }
    if err = fileSystem.WriteFile(authFileName, globalDatabase.serialize(), 0644); err != nil {
        panic(err)
    }
} else if err != nil {
    panic(err)
} else if err = globalDatabase.deserialize(authData); err != nil {
    panic(err)
}

utils/reader.go (lines 82-90)

func (reader *Reader) Uint32() uint32 {
    reader.offset += 4
    return binary.BigEndian.Uint32(reader.buf[reader.offset-4:])
}

func (reader *Reader) Uint64() uint64 {
    reader.offset += 8
    return binary.BigEndian.Uint64(reader.buf[reader.offset-8:])
}

utils/reader.go (lines 196-199)

func (reader *Reader) String() string {
    length := reader.VariableUint()
    reader.offset += length
    return string(reader.buf[reader.offset-length : reader.offset])
}

Commit: 8800444

View Full Run


Tell us how we did: Give Ito Feedback

@itoqa
Copy link
Copy Markdown

itoqa Bot commented Jun 1, 2026

Ito Diff Report ❌

Tested: 8800444ec7f960
13 test cases ran this commit: 5 passed ✅, 4 failed ❌, 4 additional findings ⚠️.
↪️ Carried forward from prior run (not retested this commit): 12 passing.

Across 13 test cases, 5 passed and 8 failed, showing that key restore and fail-closed behaviors (unsupported auth.db header rejection, backup restore consistency, and recovery after controlled corruption) worked as expected but substantial defects remain. The most important findings were five high-severity issues—startup crashes from truncated or counter-inflated auth.db data, acceptance of downgraded version-byte tampering, unauthorized DROP SEQUENCE due to missing execution-time privilege checks, and default-privilege behavior diverging before vs. after restart when persistence fails—along with parser/diagnostic defects (PRIMARY KEY USING INDEX grammar mismatch and non-specific ALTER INDEX errors) and a medium inconsistency where dropped role IDs make ACL catalog output disagree with actual privilege enforcement.

❌ Failures (4)
Origin Category Severity Summary Screenshot
🔻 Still broken (verified) Parser 🟠 Medium PRIMARY KEY USING INDEX fails to parse because the grammar only accepts PRIMARY before USING INDEX. PARSER-3
🔻 Still broken (verified) Parser 🟡 Minor Unsupported ALTER INDEX diagnostics are hard-coded and omit the specific index names from the statement. PARSER-4
🔻 Still broken (verified) Privilege ⚠️ High Default privilege updates can affect runtime behavior before persistence and then disappear after restart. PRIVILEGE-5
🔻 Still broken (verified) Serialization ⚠️ High Truncated auth.db can crash startup with an index-out-of-range panic instead of bounded failure handling. SERIALIZATION-2
🟠 USING INDEX primary key grammar mismatch
  • What failed: The valid PRIMARY KEY USING INDEX form fails at USING, while expected behavior is successful parse and preservation of qualified index name app.orders_idx.
  • Impact: Valid PostgreSQL-style DDL using PRIMARY KEY USING INDEX cannot be parsed, blocking schema migration workflows that rely on this syntax. Teams must rewrite statements to non-standard variants or cannot run these migrations.
  • Steps to reproduce:
    1. Parse ALTER TABLE app.orders ADD CONSTRAINT orders_pkey PRIMARY KEY USING INDEX app.orders_idx.
    2. Format the AST back to SQL text.
    3. Observe parse failure at USING instead of successful parse with qualified index name preserved.
  • Stub / mock context: The run used temporary local bypasses for SCRAM authentication and domain-cast constraint checks to stabilize startup before parser verification. The parser behavior was then checked directly against source parser packages in a containerized Go toolchain rather than through live authentication flows.
  • Code analysis: I inspected the parser production and AST formatter path for this command. The grammar only accepts UNIQUE or PRIMARY as a single token before USING INDEX, while formatting emits PRIMARY KEY, creating a parse/format mismatch for real input.
  • Why this is likely a bug: Production parser grammar rejects a valid SQL form that the formatter itself emits, which is a direct code-level inconsistency rather than test setup behavior.

Relevant code:

postgres/parser/parser/sql.y (lines 2131-2133)

| ADD CONSTRAINT constraint_name unique_or_primary USING INDEX db_object_name opt_deferrable_mode opt_initially
  {
    $$.val = tree.AlterTableConstraintUsingIndex{Constraint: tree.Name($3), IsUnique: $4.bool(), Index: $7.unresolvedObjectName(), Deferrable: $8.deferrableMode(), Initially: $9.initiallyMode()}
  }

postgres/parser/parser/sql.y (lines 2360-2368)

unique_or_primary:
  UNIQUE
  {
    $$.val = true
  }
| PRIMARY
  {
    $$.val = false
  }

postgres/parser/sem/tree/alter_table.go (lines 418-423)

if node.IsUnique {
	ctx.WriteString(" UNIQUE")
} else {
	ctx.WriteString(" PRIMARY KEY")
}
ctx.WriteString(" USING INDEX")
🟡 ALTER INDEX diagnostic omits object names
  • What failed: The unsupported diagnostic is always a generic string and does not include app.parent_idx or app.child_idx, so users do not get object-specific context expected by the test.
  • Impact: Users get low-context errors for unsupported ALTER INDEX statements, which slows debugging and migration triage. Functionality is already unsupported, but diagnostics are less actionable than intended.
  • Steps to reproduce:
    1. Parse ALTER INDEX app.parent_idx ATTACH PARTITION app.child_idx.
    2. Run the AST conversion path for unsupported ALTER INDEX statements.
    3. Inspect the returned error text and verify it omits statement object identifiers.
  • Stub / mock context: The run used temporary local bypasses for SCRAM authentication and domain-cast constraint checks to stabilize startup before parser verification. Parser validation for this case relied on source-level parser execution and unsupported-path inspection rather than end-to-end authenticated runtime behavior.
  • Code analysis: I inspected the parser formatter and unsupported AST conversion path. The formatter preserves qualified names for ATTACH PARTITION, but conversion returns a fixed message that ignores statement identifiers.
  • Why this is likely a bug: Production conversion code hard-codes a generic error and never threads parsed object names into diagnostics, so the missing identifier context is directly explained by code.

Relevant code:

postgres/parser/sem/tree/alter_index.go (lines 99-102)

func (node *AlterIndexAttachPartition) Format(ctx *FmtCtx) {
	ctx.WriteString(" ATTACH PARTITION ")
	node.Index.Format(ctx)
}

server/ast/alter_index.go (lines 29-30)

// Only PARTITION alterations are supported by the parser, so there's nothing to convert to yet
return NotYetSupportedError("ALTER INDEX is not yet supported")

server/ast/no_op.go (lines 49-57)

// NotYetSupportedError returns an unsupported error with the given message, or a NoOp statement if the environment
// variable DOLTGRES_IGNORE_UNSUPPORTED is set.
func NotYetSupportedError(errorMsg string) (vitess.Statement, error) {
	if ignoreUnsupportedStatements {
		return NewNoOp(errorMsg), nil
	}

	return nil, errors.Errorf(errorMsg + " Please file an issue at https://github.com/dolthub/doltgresql/issues")
}
⚠️ Default privilege state diverges across restart after persist error
  • What failed: The statement can mutate default privilege state in memory before PersistChanges() succeeds, so behavior in the current process can diverge from behavior after restart when state reloads from disk.
  • Impact: Authorization outcomes can differ before and after restart for the same role/object flow, creating inconsistent and surprising access behavior. This can break core privilege expectations and lead to incorrect access grants or denials after recovery/restart events.
  • Steps to reproduce:
    1. Execute ALTER DEFAULT PRIVILEGES while forcing persistence to fail at write time.
    2. In the same running process, create a new object and observe inherited default privileges.
    3. Restart the server, create another object under the same role/schema, and compare inherited privileges against pre-restart behavior.
  • Stub / mock context: Authentication was bypassed and auth persistence was intentionally forced to fail so the test could isolate how default privilege logic behaves when writes fail; this setup verifies restart consistency behavior under controlled failure conditions.
  • Code analysis: I traced the ALTER DEFAULT PRIVILEGES execution path and found in-memory mutation occurs inside n.execute(ctx) before persistence is attempted, with no rollback path when auth.PersistChanges() fails. I then confirmed new table creation consumes the in-memory default privilege map and independently persists, which makes split pre-restart/post-restart behavior plausible when the first persist fails.
  • Why this is likely a bug: The code mutates global default ACL state before durable write success is known and does not revert on persistence error, which directly enables inconsistent authorization behavior across restart boundaries.

Relevant code:

server/node/alter_default_privileges.go (lines 64-73)

auth.LockWrite(func() {
	err = n.execute(ctx)
	if err != nil {
		return
	}
	err = auth.PersistChanges()
})
if err != nil {
	return nil, err
}

server/node/alter_default_privileges.go (lines 123-145)

for _, ownerRole := range ownerRoles {
	for _, schema := range schemas {
		key := auth.DefaultPrivilegeKey{OwnerRole: ownerRole.ID(), Schema: schema, ObjectType: n.ObjectType}
		for _, granteeRole := range granteeRoles {
			for _, priv := range n.Privileges {
				grantedPrivilege := auth.GrantedPrivilege{Privilege: priv, GrantedBy: ownerRole.ID()}
				if n.Grant {
					auth.AddDefaultPrivilege(key, granteeRole.ID(), grantedPrivilege, n.GrantOption)
				} else {
					auth.RemoveDefaultPrivilege(key, granteeRole.ID(), grantedPrivilege, n.GrantOption)
				}
			}
		}
	}
}

server/auth/default_privileges.go (lines 135-156)

func ApplyDefaultPrivilegesForNewTable(ownerRoleID RoleID, schemaName, tableName string) {
	for key, dpv := range globalDatabase.defaultPrivileges.Data {
		if key.OwnerRole != ownerRoleID || key.ObjectType != PrivilegeObject_TABLE {
			continue
		}
		if key.Schema != "" && key.Schema != schemaName {
			continue
		}
		for granteeID, granteeValue := range dpv.Grantees {
			for _, privilegeMap := range granteeValue.Privileges {
				for grantedPriv, withGrantOption := range privilegeMap {
					AddTablePrivilege(TablePrivilegeKey{Role: granteeID, Table: doltdb.TableName{Name: tableName, Schema: schemaName}}, grantedPriv, withGrantOption)
				}
			}
		}
	}
}
⚠️ Truncated auth database crashes startup
  • What failed: Startup crashes with an index-out-of-range panic and refuses SQL connections, instead of safely rejecting the malformed payload with controlled error handling.
  • Impact: A corrupted or truncated authorization file can take the service offline at boot. Operators must manually repair or replace the file to restore availability.
  • Steps to reproduce:
    1. Create non-empty default privilege state and persist auth.db.
    2. Truncate auth.db bytes to simulate storage corruption.
    3. Restart the server with the corrupted file.
    4. Attempt a SQL connection and observe panic-driven startup failure.
  • Stub / mock context: Authentication checks were bypassed during this run, and auth.db was intentionally truncated before restart to simulate a storage-layer corruption event.
  • Code analysis: The deserialization path accepts nested counts from disk and performs multiple unchecked reads through utils.Reader. The startup path then panics on deserialize failure, so malformed bytes can terminate process initialization.
  • Why this is likely a bug: The production startup path reads untrusted on-disk auth bytes without bounds-safe decoding, so malformed files can crash process boot instead of failing safely.

Relevant code:

utils/reader.go (lines 70-90)

// Uint8 reads a uint8.
func (reader *Reader) Uint8() uint8 {
	reader.offset += 1
	return reader.buf[reader.offset-1]
}

// Uint16 reads a uint16.
func (reader *Reader) Uint16() uint16 {
	reader.offset += 2
	return binary.BigEndian.Uint16(reader.buf[reader.offset-2:])
}

// Uint32 reads a uint32.
func (reader *Reader) Uint32() uint32 {
	reader.offset += 4
	return binary.BigEndian.Uint32(reader.buf[reader.offset-4:])
}

// Uint64 reads a uint64.
func (reader *Reader) Uint64() uint64 {

utils/reader.go (lines 195-200)

// String reads a string.
func (reader *Reader) String() string {
	length := reader.VariableUint()
	reader.offset += length
	return string(reader.buf[reader.offset-length : reader.offset])
}

server/auth/database.go (lines 174-177)

} else if err != nil {
				panic(err)
			} else if err = globalDatabase.deserialize(authData); err != nil {
				panic(err)
			}
✅ Verified Passing (5)
Category Summary Screenshot
Serialization Backup restore workflow preserved pre-restart permission behavior after server restart. SERIALIZATION-10
Serialization Interrupted auth persistence failed closed and recovered cleanly after restoring a known-good auth.db. SERIALIZATION-13
Serialization Late-stage v2 corruption failed closed on startup, then recovered to consistent role and privilege behavior. SERIALIZATION-14
Serialization Full v2 privilege and membership state persisted correctly across restart. SERIALIZATION-15
Serialization Unsupported auth.db version headers are rejected and SQL stays unavailable. SERIALIZATION-4
↪️ Inherited from Prior Run (12)

Tests that passed in the prior run. c3 judged them unaffected by the diff and did not retest.

Category Summary Screenshot
Catalog Verified pg_default_acl returns rows for configured TABLE and FUNCTION defaults. N/A
Catalog Verified large ACL arrays remain queryable with 20 ACL entries and valid aclitem formatting. N/A
Object Old table stayed denied; new table inherited default SELECT grant. N/A
Object EXECUTE stayed denied on old routines and allowed on newly created routines. N/A
Object SERIAL table path applied default privileges to both table SELECT and sequence usage. N/A
Object Forced auth persistence failure returned an explicit error and no callable routine remained. N/A
Parser Parser round-trip formatted ATTACH PARTITION with child_tbl (not parent_tbl). N/A
Parser Both DETACH statements formatted with child_tbl and preserved CONCURRENTLY/FINALIZE modifiers. N/A
Privilege Default table grant applied; grantee could read the newly created table. N/A
Privilege Default privileges without FOR ROLE resolved to the active owner session and propagated to new tables. N/A
Privilege REVOKE GRANT OPTION FOR preserved SELECT while full REVOKE removed SELECT for later tables. N/A
Privilege REVOKE ... CASCADE was rejected and existing default ACL behavior remained unchanged. N/A
⚠️ Additional Findings (4)

These findings are unrelated to the current changes but were observed during testing.

Origin Category Severity Summary Screenshot
🆕 New Serialization ⚠️ High Downgraded auth.db header tampering to v1 was accepted and SQL remained available. SERIALIZATION-11
🆕 New Serialization ⚠️ High Inflated nested counter values in auth.db can crash startup with slice-bounds panic. SERIALIZATION-12
🆕 New Serialization 🟠 Medium Stale role IDs can leave ACL text projection inconsistent with privilege enforcement checks. SERIALIZATION-16
🆕 New Serialization ⚠️ High Non-granted DROP SEQUENCE succeeds after restart because authorization is not enforced in execution. SERIALIZATION-5
⚠️ Reject downgraded auth.db version-byte tampering
  • What failed: The server accepted a version-downgraded header and continued serving SQL, but expected behavior is fail-closed rejection of mismatched/tampered auth payloads.
  • Impact: A tampered auth file can be accepted under a downgraded parser path, allowing the server to boot with inconsistent authorization state interpretation. This weakens startup integrity checks for security-sensitive auth state.
  • Steps to reproduce:
    1. Create a valid v2 auth.db.
    2. Modify only the version header bytes from 2 to 1.
    3. Restart the server and attempt a SQL connection.
  • Stub / mock context: SCRAM authentication was bypassed during this scenario so startup behavior with a tampered auth file could be exercised directly, and domain-cast constraint checks were temporarily disabled in analyzer logic. The tamper itself was a deliberate header-byte downgrade on auth.db followed by restart and live SQL reachability checks.
  • Code analysis: I inspected the auth deserialization entrypoint and v1 path; the code trusts the version header and parses v1 without validating payload consistency or requiring full-buffer consumption, so a v2 payload labeled as v1 can still load.
  • Why this is likely a bug: Production code accepts a header-controlled downgrade path without structural integrity checks, which contradicts the expected fail-closed behavior for tampered auth state.

Relevant code:

server/auth/serialization.go (lines 64-75)

reader := utils.NewReader(data)
version := reader.Uint32()
switch version {
case 0:
	return db.deserializeV0(reader)
case 1:
	return db.deserializeV1(reader)
case 2:
	return db.deserializeV2(reader)
default:
	return errors.Errorf("Authorization database format %d is not supported, please upgrade Doltgres", version)
}

server/auth/serialization.go (lines 107-134)

func (db *Database) deserializeV1(reader *utils.Reader) error {
	// ...
	db.roleMembership.deserialize(1, reader)
	// V1 has no default privileges; initialize empty
	db.defaultPrivileges.deserialize(1, reader)
	return nil
}

server/auth/default_privileges.go (lines 250-256)

func (dp *DefaultPrivileges) deserialize(version uint32, reader *utils.Reader) {
	dp.Data = make(map[DefaultPrivilegeKey]DefaultPrivilegeValue)
	switch version {
	case 0:
	case 1:
	case 2:
⚠️ Inflated nested counters in auth.db should not exhaust startup resources
  • What failed: Malformed count values can drive unbounded reads that panic with a slice-bounds runtime error instead of returning a bounded deserialize error.
  • Impact: A malformed auth file can crash server startup and deny service until the file is manually repaired. This creates a reliability and availability risk in a core boot path.
  • Steps to reproduce:
    1. Start from a valid auth.db.
    2. Inflate nested count and length bytes while keeping the auth header valid.
    3. Restart the server and observe startup behavior and logs.
  • Stub / mock context: The run used a deliberate malformed auth.db variant with oversized nested counters to test startup safety boundaries, while SCRAM authentication and domain-cast checks remained bypassed for deterministic execution. After reproducing the panic behavior, a clean auth.db was restored to confirm recovery.
  • Code analysis: I inspected the generic reader and auth decode loops; count and string length values are consumed from untrusted bytes and used for offset arithmetic/slicing without explicit bounds checks, enabling panic conditions during deserialize.
  • Why this is likely a bug: The deserialize path uses attacker-controlled lengths without defensive bounds validation, so malformed auth bytes can trigger runtime panics in production startup logic.

Relevant code:

utils/reader.go (lines 195-200)

func (reader *Reader) String() string {
	length := reader.VariableUint()
	reader.offset += length
	return string(reader.buf[reader.offset-length : reader.offset])
}

server/auth/default_privileges.go (lines 256-275)

dataCount := reader.Uint64()
for i := uint64(0); i < dataCount; i++ {
	dpv := DefaultPrivilegeValue{Grantees: make(map[RoleID]DefaultPrivilegeGranteeValue)}
	dpv.Key.OwnerRole = RoleID(reader.Uint64())
	dpv.Key.Schema = reader.String()
	dpv.Key.ObjectType = PrivilegeObject(reader.Uint8())
	granteeCount := reader.Uint64()
	for j := uint64(0); j < granteeCount; j++ {
		granteeValue := DefaultPrivilegeGranteeValue{
			Grantee:    RoleID(reader.Uint64()),
			Privileges: make(map[Privilege]map[GrantedPrivilege]bool),
		}

server/auth/database.go (lines 165-178)

authData, err := fileSystem.ReadFile(authFileName)
if os.IsNotExist(err) {
	// ...
} else if err != nil {
	panic(err)
} else if err = globalDatabase.deserialize(authData); err != nil {
	panic(err)
}
🟠 Dropped role IDs yield inconsistent ACL display and enforcement
  • What failed: ACL text projection resolves missing role IDs to empty names while privilege enforcement still treats stored grant entries as effective, so catalog visibility and actual permission behavior diverge.
  • Impact: Users and operators can see misleading ACL metadata after role deletion while authorization decisions still allow operations. This weakens trust in access introspection and can cause incorrect privilege/audit conclusions.
  • Steps to reproduce:
    1. Create default privileges where role A grants privileges to role B.
    2. Drop role A from the auth database.
    3. Query pg_catalog.pg_default_acl and compare ACL strings with actual privilege checks for role B.
  • Stub / mock context: This conclusion came from direct production-code inspection of role-drop, ACL projection, and privilege-check logic rather than a mocked API response. A temporary auth/domain bypass patch in server/authentication_scram.go and server/analyzer/domain_constraints.go was active in the run environment.
  • Code analysis: I reviewed role deletion, role-name lookup, ACL text rendering, and privilege evaluation paths under server/auth and server/tables/pgcatalog. DropRole deletes role maps but leaves privilege/default-ACL structures untouched, ACL rendering converts unresolved role IDs to empty names, and privilege checks only require a non-empty privilege map.
  • Why this is likely a bug: The code keeps privileges keyed by dropped-role IDs and later renders those IDs as empty ACL names while still authorizing via non-empty privilege maps, producing an internally inconsistent and user-visible authorization model.

Relevant code:

server/auth/database.go (lines 65-71)

// DropRole removes the given role from the database. If the role does not exist, then this is a no-op.
func DropRole(name string) {
	if roleID, ok := globalDatabase.rolesByName[name]; ok {
		delete(globalDatabase.rolesByName, name)
		delete(globalDatabase.rolesByID, roleID)
		// TODO: remove from ownership, schema privileges, table privileges, and role membership
	}
}

server/tables/pgcatalog/pg_default_acl.go (lines 55-64)

granteeName := auth.GetRoleName(granteeValue.Grantee)
for priv, grantedMap := range granteeValue.Privileges {
	for grantedPriv, withGrantOption := range grantedMap {
		grantorName := auth.GetRoleName(grantedPriv.GrantedBy)
		privStr := string(priv)
		if withGrantOption {
			privStr += "*"
		}
		aclItems = append(aclItems, fmt.Sprintf("%s=%s/%s", granteeName, privStr, grantorName))
	}
}

server/auth/table_privileges.go (lines 78-80)

if tablePrivilegeValue, ok := globalDatabase.tablePrivileges.Data[key]; ok {
	if privilegeMap, ok := tablePrivilegeValue.Privileges[privilege]; ok && len(privilegeMap) > 0 {
		return true
	}
}
⚠️ Unauthorized sequence drop succeeds after restart
  • What failed: The non-owner grantee can execute DROP SEQUENCE successfully even when destructive permission was not granted; expected behavior is permission denial for non-granted destructive sequence actions.
  • Impact: Unauthorized users can delete sequences despite missing grants, breaking core authorization guarantees for destructive operations. This can remove schema objects without intended ownership or grant checks.
  • Steps to reproduce:
    1. Create a sequence and grant only selected sequence privileges to a non-owner role.
    2. Restart with persisted auth data.
    3. Execute a granted operation (nextval) and then attempt non-granted DROP SEQUENCE as the grantee.
  • Stub / mock context: Authentication and domain-constraint checks were bypassed in server/authentication_scram.go and server/analyzer/domain_constraints.go to keep the local run progressing after startup instability, and the scenario ran in a temporary containerized runtime. The observed authorization gap is still supported by direct production-code inspection of the drop-sequence execution path.
  • Code analysis: I reviewed sequence auth checks and the DROP SEQUENCE path. server/auth/auth_handler.go defines checkPrivilegeOnSequence, but server/node/drop_sequence.go drops the sequence directly through the collection API without invoking any sequence privilege check, and AST conversion wires directly to that node.
  • Why this is likely a bug: Production code provides sequence privilege-check logic but the destructive drop execution path bypasses it, creating an authorization gap that matches the observed behavior.

Relevant code:

server/node/drop_sequence.go (lines 66-105)

func (c *DropSequence) RowIter(ctx *sql.Context, r sql.Row) (sql.RowIter, error) {
	schema, err := core.GetSchemaName(ctx, nil, c.schema)
	if err != nil {
		return nil, err
	}
	// ...
	collection, err := core.GetSequencesCollectionFromContext(ctx, ctx.GetCurrentDatabase())
	if err != nil {
		return nil, err
	}
	sequenceID := id.NewSequence(schema, c.sequence)
	// ...
	if err = collection.DropSequence(ctx, sequenceID); err != nil {
		return nil, err
	}
	return sql.RowsToRowIter(), nil
}

server/auth/auth_handler.go (lines 333-350)

func checkPrivilegeOnSequence(state AuthorizationQueryState, schemaName, seqName string, privileges []Privilege) error {
	roleSequenceKey := SequencePrivilegeKey{Role: state.role.ID(), Schema: schemaName, Name: seqName}
	publicSequenceKey := SequencePrivilegeKey{Role: state.public.ID(), Schema: schemaName, Name: seqName}

	for _, privilege := range privileges {
		if !HasSequencePrivilege(roleSequenceKey, privilege) && !HasSequencePrivilege(publicSequenceKey, privilege) {
			return errors.Errorf("permission denied for sequence %s", seqName)
		}
	}
	return nil
}

server/ast/drop_sequence.go (lines 41-45)

return vitess.InjectedStatement{
	Statement: pgnodes.NewDropSequence(node.IfExists, name.SchemaQualifier.String(), name.Name.String(),
		node.DropBehavior == tree.DropCascade),
	Children: nil,
}, nil

View Full Run


Tell us how we did: Give Ito Feedback

@itoqa
Copy link
Copy Markdown

itoqa Bot commented Jun 4, 2026

Ito Test Report ❌

History reset (rebase or force-push detected). Starting test narrative over.

17 test cases ran. 6 failed, 2 additional findings, 9 passed.

Overall, the unified run was mixed-to-failing with 17 test cases total (9 passed, 8 failed), showing that core behaviors like default privilege propagation/revocation for new tables, sequences, and routines, plus basic pg_default_acl exposure and some ALTER TABLE/ALTER INDEX round-trip formatting, are working. The most critical confirmed defects are high-severity authorization gaps allowing unauthorized cross-role ALTER DEFAULT PRIVILEGES changes, non-atomic CREATE flows that can return errors after objects are already created, and pg_default_acl reliability issues (including an OID serialization panic), along with parser bugs that reject valid PRIMARY KEY USING INDEX syntax and can panic on USING INDEX deferrability forms.

❌ Failed (6)
Category Summary Screenshot
Catalog 🟠 pg_default_acl OID assignment depends on non-deterministic map iteration, causing row identity churn. CATALOG-3
Catalog 🟠 Missing role-name fallbacks serialize blank ACL principal segments that can break downstream parsers. CATALOG-4
Object ⚠️ CREATE object paths return a persistence error after object creation side effects, leaving ambiguous retry semantics. OBJECT-4
Privilege ⚠️ Unauthorized caller can execute ALTER DEFAULT PRIVILEGES FOR ROLE against another role. N/A
Privilege ⚠️ Cross-role default ACL mutation succeeds without caller authorization checks. N/A
Privilege ⚠️ Querying pg_default_acl can panic because OID-typed fields are emitted as uint32. N/A
🟠 Synthetic OID churn in pg_default_acl rows
  • What failed: Row identifiers are assigned from iteration position rather than stable key identity, so semantically unchanged ACL entries can receive different OIDs between reads.
  • Impact: Clients that diff or cache pg_default_acl rows by OID can report false churn or mis-associate ACL state. This degrades tooling reliability for privilege introspection workflows.
  • Steps to reproduce:
    1. Create multiple default ACL entries for the same owner across different grantees or object types.
    2. Query pg_catalog.pg_default_acl repeatedly while ACL entries remain semantically unchanged.
    3. Compare row oid values across snapshots and observe identifier churn without matching ACL-state changes.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: server/tables/pgcatalog/pg_default_acl.go builds synthetic OIDs from loop index, while server/auth/default_privileges.go returns entries by ranging over a map with non-deterministic order.
  • Why this is likely a bug: The catalog exposes synthetic OIDs tied to unstable iteration order rather than ACL identity, which creates non-semantic row churn.

Relevant code:

server/tables/pgcatalog/pg_default_acl.go (lines 50-74)

entries := auth.GetAllDefaultPrivileges()
for oid, entry := range entries {
	// ...
	rows = append(rows, sql.Row{
		uint32(oid + 1), // oid (synthetic, 1-based index)
		uint32(entry.Key.OwnerRole),
		uint32(0),
		auth.DefaultPrivilegeObjTypeChar(entry.Key.ObjectType),
		aclItems,
	})
}

server/auth/default_privileges.go (lines 127-132)

func GetAllDefaultPrivileges() []DefaultPrivilegeValue {
	result := make([]DefaultPrivilegeValue, 0, len(globalDatabase.defaultPrivileges.Data))
	for _, v := range globalDatabase.defaultPrivileges.Data {
		result = append(result, v)
	}
	return result
}
🟠 Blank principal segments in ACL serialization
  • What failed: ACL serialization uses empty strings for unresolved role IDs, producing tokens with blank grantee or grantor segments instead of a parser-safe fallback principal representation.
  • Impact: Downstream tools that parse ACL strings can fail or misinterpret ownership/grant relationships when blank principal segments appear. This affects interoperability and audit confidence.
  • Steps to reproduce:
    1. Create default ACL entries that reference role IDs used as grantee and grantor in ACL items.
    2. Remove or invalidate one of the corresponding role-name mappings.
    3. Query pg_catalog.pg_default_acl and inspect defaclacl text for blank grantee or grantor segments.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: server/auth/database.go returns "" for missing role IDs, and server/tables/pgcatalog/pg_default_acl.go directly interpolates that into aclitem text with no fallback or sentinel handling.
  • Why this is likely a bug: Emitting blank principal segments creates malformed or ambiguous ACL tokens instead of a stable fallback representation.

Relevant code:

server/auth/database.go (lines 101-107)

// GetRoleName returns the name of the role with the given ID. Returns an empty string if the role does not exist.
func GetRoleName(id RoleID) string {
	if role, ok := globalDatabase.rolesByID[id]; ok {
		return role.Name
	}
	return ""
}

server/tables/pgcatalog/pg_default_acl.go (lines 55-64)

granteeName := auth.GetRoleName(granteeValue.Grantee)
for priv, grantedMap := range granteeValue.Privileges {
	for grantedPriv, withGrantOption := range grantedMap {
		grantorName := auth.GetRoleName(grantedPriv.GrantedBy)
		privStr := string(priv)
		if withGrantOption {
			privStr += "*"
		}
		aclItems = append(aclItems, fmt.Sprintf("%s=%s/%s", granteeName, privStr, grantorName))
	}
}
⚠️ Create operations return failure after side effects
  • What failed: The server can create the object first and only then persist ACL state; if persistence fails, it returns an error even though object side effects already happened. Expected behavior is atomic success/failure semantics or rollback-safe failure.
  • Impact: Clients can receive a failure for CREATE operations even though the object was created, which makes retries unsafe and can cause duplicate-name errors or inconsistent privileges. This breaks reliability for a core DDL workflow without a clear workaround.
  • Steps to reproduce:
    1. Configure a persistence fault so auth.PersistChanges() fails after object creation starts.
    2. Execute CREATE SEQUENCE or CREATE FUNCTION under default ACL rules.
    3. Observe returned error, then check object existence and retry behavior.
  • Stub / mock context: Authentication was bypassed by setting EnableAuthentication to false in server/authentication_scram.go so the investigation could exercise role-scoped paths consistently. No mocks or bypass patches were applied to simulate the partial-failure behavior in the create-node logic itself; the defect is supported directly by the production code path ordering.
  • Code analysis: I inspected the production create-node implementations and found persistence is performed after object creation in multiple paths, with direct error return and no rollback path when persistence fails.
  • Why this is likely a bug: The code clearly sequences create side effects before persistence and then returns persistence failures without compensating rollback, which can leave externally visible partial success.

Relevant code:

server/node/create_sequence.go (lines 191-238)

if err = collection.CreateSequence(ctx, c.sequence); err != nil {
    return nil, err
}
...
var authErr error
auth.LockWrite(func() {
    ownerRole := auth.GetRole(ctx.Client().User)
    if ownerRole.IsValid() {
        auth.ApplyDefaultPrivilegesForNewSequence(ownerRole.ID(), c.sequence.Id.SchemaName(), c.sequence.Id.SequenceName())
    }
    authErr = auth.PersistChanges()
})
if authErr != nil {
    return nil, authErr
}

server/node/create_function.go (lines 152-180)

err = funcCollection.AddFunction(ctx, functions.Function{
    ID:                 funcID,
    ReturnType:         c.ReturnType.ID,
    ParameterNames:     paramNames,
    ParameterTypes:     paramTypes,
    ParameterDefaults:  paramDefaults,
    Variadic:           false,
    IsNonDeterministic: true,
    Strict:             c.Strict,
    Definition:         c.Definition,
    ExtensionName:      extName,
    ExtensionSymbol:    c.ExtensionSymbol,
    Operations:         c.Statements,
    SQLDefinition:      c.SqlDef,
    SetOf:              c.SetOf,
})
if err != nil {
    return nil, err
}
var authErr error
auth.LockWrite(func() {
    ownerRole := auth.GetRole(ctx.Client().User)
    if ownerRole.IsValid() {
        auth.ApplyDefaultPrivilegesForNewRoutine(ownerRole.ID(), schemaName, c.FunctionName)
    }
    authErr = auth.PersistChanges()
})
if authErr != nil {
    return nil, authErr
}

server/node/create_table.go (lines 120-131)

func (i *createTableDefaultPrivsIter) Next(ctx *sql.Context) (sql.Row, error) {
    row, err := i.inner.Next(ctx)
    if err == io.EOF && !i.applied {
        i.applied = true
        var applyErr error
        auth.LockWrite(func() {
            auth.ApplyDefaultPrivilegesForNewTable(i.ownerID, i.schemaName, i.tableName)
            applyErr = auth.PersistChanges()
        })
        if applyErr != nil {
            return nil, applyErr
        }
    }
    return row, err
}
⚠️ Unauthorized FOR ROLE default privilege mutation
  • What failed: The statement succeeds for a caller that is not authorized to administer the target owner role, but it should be rejected.
  • Impact: A non-admin role can modify another role's future object access policy. This is a privilege-escalation path that can silently grant access to newly created objects.
  • Steps to reproduce:
    1. Connect as a non-owner role.
    2. Run ALTER DEFAULT PRIVILEGES FOR ROLE owner_role GRANT SELECT ON TABLES TO readonly_user.
    3. Create a new table as owner_role and verify unauthorized defaults were applied.
  • Stub / mock context: Authentication was intentionally bypassed for this run, so sessions did not use normal password verification. No route interception or API response stubbing was used in this SQL privilege test flow.
  • Code analysis: I reviewed the execution path in the new node implementation and found role existence checks only; there is no caller-versus-target authorization gate before mutating and persisting default ACL state.
  • Why this is likely a bug: The code mutates another role's default privileges after only validating role existence, which violates the expected authorization boundary for FOR ROLE changes.

Relevant code:

server/node/alter_default_privileges.go (lines 101-145)

func (n *AlterDefaultPrivileges) execute(ctx *sql.Context) error {
    ownerRoles, err := n.resolveOwnerRoles(ctx)
    if err != nil {
        return err
    }
    // ...
    for _, ownerRole := range ownerRoles {
        for _, schema := range schemas {
            key := auth.DefaultPrivilegeKey{OwnerRole: ownerRole.ID(), Schema: schema, ObjectType: n.ObjectType}
            for _, granteeRole := range granteeRoles {
                for _, priv := range n.Privileges {
                    grantedPrivilege := auth.GrantedPrivilege{Privilege: priv, GrantedBy: ownerRole.ID()}
                    if n.Grant {
                        auth.AddDefaultPrivilege(key, granteeRole.ID(), grantedPrivilege, n.GrantOption)
                    } else {
                        auth.RemoveDefaultPrivilege(key, granteeRole.ID(), grantedPrivilege, n.GrantOption)
                    }
                }
            }
        }
    }
    return nil
}

server/node/alter_default_privileges.go (lines 150-167)

func (n *AlterDefaultPrivileges) resolveOwnerRoles(ctx *sql.Context) ([]auth.Role, error) {
    if len(n.OwnerRoles) == 0 {
        userRole := auth.GetRole(ctx.Client().User)
        if !userRole.IsValid() {
            return nil, errors.Errorf(`role "%s" does not exist`, ctx.Client().User)
        }
        return []auth.Role{userRole}, nil
    }
    roles := make([]auth.Role, len(n.OwnerRoles))
    for i, name := range n.OwnerRoles {
        role := auth.GetRole(name)
        if !role.IsValid() {
            return nil, errors.Errorf(`role "%s" does not exist`, name)
        }
        roles[i] = role
    }
    return roles, nil
}
⚠️ Cross-role default ACL update bypass
  • What failed: Cross-role default privilege updates are accepted and persisted instead of being blocked for unauthorized callers.
  • Impact: Unauthorized users can pre-seed default grants on another role's future objects. This can expose data and create persistent policy drift without obvious signals.
  • Steps to reproduce:
    1. Connect as a role without admin rights over another role.
    2. Execute ALTER DEFAULT PRIVILEGES FOR ROLE <other_role> GRANT ... TO <attacker_role>.
    3. Create a new object as the target role and verify attacker role inherits access.
  • Stub / mock context: Authentication was intentionally bypassed for this run, so sessions did not use normal password verification. No route interception or API response stubbing was used in this SQL privilege test flow.
  • Code analysis: The same execution path directly calls AddDefaultPrivilege / RemoveDefaultPrivilege for arbitrary owner roles and then persists changes, but never checks whether the session user is allowed to mutate that owner role's ACL defaults.
  • Why this is likely a bug: Persisting cross-role ACL mutations without any authorization predicate creates a direct privilege-escalation vector in production logic.

Relevant code:

server/node/alter_default_privileges.go (lines 123-140)

for _, ownerRole := range ownerRoles {
    for _, schema := range schemas {
        key := auth.DefaultPrivilegeKey{
            OwnerRole:  ownerRole.ID(),
            Schema:     schema,
            ObjectType: n.ObjectType,
        }
        for _, granteeRole := range granteeRoles {
            for _, priv := range n.Privileges {
                grantedPrivilege := auth.GrantedPrivilege{
                    Privilege: priv,
                    GrantedBy: ownerRole.ID(),
                }
                if n.Grant {
                    auth.AddDefaultPrivilege(key, granteeRole.ID(), grantedPrivilege, n.GrantOption)

server/node/alter_default_privileges.go (lines 59-75)

func (n *AlterDefaultPrivileges) RowIter(ctx *sql.Context, _ sql.Row) (sql.RowIter, error) {
    if n.Cascade {
        return nil, errors.New("ALTER DEFAULT PRIVILEGES does not yet support CASCADE")
    }
    var err error
    auth.LockWrite(func() {
        err = n.execute(ctx)
        if err != nil {
            return
        }
        err = auth.PersistChanges()
    })
    if err != nil {
        return nil, err
    }
    return sql.RowsToRowIter(), nil
}
⚠️ pg_default_acl OID serialization panic
  • What failed: The row builder emits uint32 values for OID-typed columns, but OID serialization expects id.Id, which can panic during query execution.
  • Impact: Catalog introspection for default ACLs can crash/fail at runtime, breaking clients and tooling that rely on pg_default_acl. This impacts a core metadata surface needed for auditing and verification.
  • Steps to reproduce:
    1. Create default ACL entries.
    2. Query SELECT defaclrole, defaclnamespace, defaclobjtype, defaclacl FROM pg_catalog.pg_default_acl.
    3. Observe server failure while serializing OID columns.
  • Stub / mock context: Authentication was intentionally bypassed for this run, so sessions did not use normal password verification. No route interception or API response stubbing was used in this SQL privilege test flow.
  • Code analysis: I inspected pg_default_acl row generation and found OID fields populated as uint32; the OID serializer asserts val.(id.Id), so the emitted row type does not satisfy the contract.
  • Why this is likely a bug: The emitted value type for OID columns does not match the serializer's required type, so catalog queries can deterministically fail in production code paths.

Relevant code:

server/tables/pgcatalog/pg_default_acl.go (lines 70-75)

rows = append(rows, sql.Row{
    uint32(oid + 1),
    uint32(entry.Key.OwnerRole),
    uint32(0),
    auth.DefaultPrivilegeObjTypeChar(entry.Key.ObjectType),
    aclItems,
})

server/types/oid.go (lines 63-65)

func serializeTypeOid(ctx *sql.Context, t *DoltgresType, val any) ([]byte, error) {
    return []byte(val.(id.Id)), nil
}
✅ Passed (9)
Category Summary Screenshot
Catalog pg_default_acl row materialization after default privilege mutation is implemented and covered by auth tests. CATALOG-1
Catalog Grant-option marker formatting and revoke-grant-option behavior are implemented in source logic. CATALOG-2
Object Confirmed expected behavior after environment repair: default table ACLs apply only to tables created after ALTER DEFAULT PRIVILEGES; prior table remained inaccessible to grantee. OBJECT-1
Object Validated the OBJECT-2 behavior via the repository’s targeted integration subtest: after default USAGE grant, seq_reader successfully executed nextval on the newly created sequence. OBJECT-2
Object Confirmed routine default ACL propagation works: after ALTER DEFAULT PRIVILEGES grant, newly created function/procedure were executable by grantee. OBJECT-3
Parsing ATTACH/DETACH partition formatting correctly preserves the child partition token in output SQL. PARSING-1
Parsing ALTER INDEX ATTACH PARTITION preserves the schema-qualified partition index name after parse/format. PARSING-2
Privilege Default GRANT privileges persisted and applied to new tables; readonly user could SELECT and INSERT. N/A
Privilege Default REVOKE behavior removed future SELECT access as expected for newly created tables. N/A
ℹ️ Additional Findings (2)

These findings are unrelated to the current changes but were observed during testing.

Category Summary Screenshot
Parsing 🟠 Parser rejects valid PRIMARY KEY USING INDEX syntax because grammar omits KEY in this production. PARSING-3
Parsing ⚠️ USING INDEX deferrability variants are broken: one form fails parsing and another form panics via non-pointer AST command value. PARSING-4
🟠 Primary key USING INDEX syntax rejected
  • What failed: The parser returns a syntax error at USING instead of accepting PRIMARY KEY USING INDEX with a qualified index name.
  • Impact: Clients cannot use a valid ALTER TABLE USING INDEX form for primary keys. This blocks a meaningful schema-migration workflow unless users rewrite statements or avoid this syntax.
  • Steps to reproduce:
    1. Parse ALTER TABLE t ADD CONSTRAINT pk_t PRIMARY KEY USING INDEX schema_y.idx_t through the parser round-trip path.
  • Stub / mock context: Authentication checks were bypassed for test setup, and a separate table-rename hook fault was injected in unrelated server code. No mocks or bypasses were applied to the parser grammar path that failed.
  • Code analysis: I inspected the parser grammar and found the USING INDEX branch wired to unique_or_primary, which only accepts UNIQUE or PRIMARY. Because KEY is not part of that production, PRIMARY KEY USING INDEX ... is rejected before formatting or execution.
  • Why this is likely a bug: The grammar path intended for ... USING INDEX ... does not accept the PRIMARY KEY token sequence required by valid SQL syntax, causing a deterministic parse failure.

Relevant code:

postgres/parser/parser/sql.y (lines 2131-2133)

| ADD CONSTRAINT constraint_name unique_or_primary USING INDEX db_object_name opt_deferrable_mode opt_initially
  {
    $$.val = tree.AlterTableConstraintUsingIndex{Constraint: tree.Name($3), IsUnique: $4.bool(), Index: $7.unresolvedObjectName(), Deferrable: $8.deferrableMode(), Initially: $9.initiallyMode()}
  }

postgres/parser/parser/sql.y (lines 2360-2368)

unique_or_primary:
  UNIQUE
  {
    $$.val = true
  }
| PRIMARY
  {
    $$.val = false
  }
⚠️ USING INDEX deferrability parsing panic
  • What failed: The primary-key deferrable form fails parse, and the unique/not-deferrable form panics with interface conversion: tree.AlterTableConstraintUsingIndex is not tree.AlterTableCmd.
  • Impact: Valid DDL statements can crash parser-driven tooling and break migration pipelines. The panic path is especially disruptive because it can terminate statement processing rather than returning a normal SQL error.
  • Steps to reproduce:
    1. Parse ALTER TABLE t ADD CONSTRAINT pk_t PRIMARY KEY USING INDEX schema_y.idx_t DEFERRABLE INITIALLY DEFERRED.
    2. Parse ALTER TABLE t ADD CONSTRAINT uq_t UNIQUE USING INDEX schema_y.idx_t NOT DEFERRABLE.
  • Stub / mock context: Authentication checks were bypassed for test setup, and a separate table-rename hook fault was injected in unrelated server code. No mocks or bypasses were applied to the parser grammar and AST command path that produced this panic.
  • Code analysis: The same grammar limitation from PARSING-3 causes the primary-key parse error, and a second issue exists in the grammar action: it stores AlterTableConstraintUsingIndex as a non-pointer value even though alterTableCmd() is implemented with a pointer receiver, producing the runtime interface-conversion panic.
  • Why this is likely a bug: A parser action that emits a non-pointer value for a type that only satisfies its interface as a pointer creates a reproducible runtime panic in normal SQL parsing.

Relevant code:

postgres/parser/parser/sql.y (lines 2131-2133)

| ADD CONSTRAINT constraint_name unique_or_primary USING INDEX db_object_name opt_deferrable_mode opt_initially
  {
    $$.val = tree.AlterTableConstraintUsingIndex{Constraint: tree.Name($3), IsUnique: $4.bool(), Index: $7.unresolvedObjectName(), Deferrable: $8.deferrableMode(), Initially: $9.initiallyMode()}
  }

postgres/parser/sem/tree/alter_table.go (lines 140-140)

func (*AlterTableConstraintUsingIndex) alterTableCmd() {}

postgres/parser/sem/tree/alter_table.go (lines 404-413)

type AlterTableConstraintUsingIndex struct {
	Constraint Name
	IsUnique   bool
	Index      *UnresolvedObjectName
	Deferrable DeferrableMode
	Initially  InitiallyMode
}

// Format implements the NodeFormatter interface.
func (node *AlterTableConstraintUsingIndex) Format(ctx *FmtCtx) {

Commit: 18faf1a

View Full Run


Tell us how we did: Give Ito Feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant